build: use placeholder version for stamping#22467
build: use placeholder version for stamping#22467dgp1130 merged 2 commits intoangular:masterfrom aspect-build:version-stamp
Conversation
|
This is nice in that it makes it clear what is being substituted where and also brings this repo in like with the substitution strings used in angular/angular. A few questions @kormide,
|
| # Version of the local package being built, generated via the `--workspace_status_command` flag. | ||
| "0.0.0-PLACEHOLDER": "{BUILD_SCM_VERSION}", | ||
| # Versions to declare in release `package.json`s engines field | ||
| "0.0.0-ENGINES-NODE": "^12.20.0 || ^14.15.0 || >=16.10.0", |
There was a problem hiding this comment.
Was hoisting the versions to here so that they are more visible as opposed to being hidden in the filter file?
There was a problem hiding this comment.
I suspect its better to not add a new top-level file named substitutions.bzl as the substitutions are implementation details of pkg_npm; perhaps a top-level constants.bzl which defines all the constants and then use those constants in tools/defaults.bzl. Something like
RELEASE_ENGINES_NODE = "^12.20.0 || ^14.15.0 || >=16.10.0"
RELEASE_ENGINES_NPM = "^6.11.0 || ^7.5.6 || >=8.0.0"
RELEASE_ENGINES_YARN = ">= 1.13.0"
A follow-up could define some constants for WORKSPACE as well since there are node versions & hard-coded there.
There was a problem hiding this comment.
I added this in response to this feedback on the previous PR: #22425 (comment). The angular components repo has a top-level bazel file with some version substitutions.
I could make it a file with just constants. Wdyt @devversion?
There was a problem hiding this comment.
Oh. You borrowed the pattern from components. That makes sense. I looked in angular/angular and didn't see any top level .bzl files. Their subs are defined in tools/defaults.bzl https://github.com/angular/angular/blob/master/tools/defaults.bzl#L254
There was a problem hiding this comment.
There was a problem hiding this comment.
I like constants.bzl as its clear what the file is meant for but that is just bike shedding at that point
There was a problem hiding this comment.
Renamed to constants.bzl and moved substitution logic into tools/defaults.bzl.
There was a problem hiding this comment.
Thank you! yeah I personally like the top-level file as this is something that folks can change without needing a deep understanding of the Bazel setup. Constants sounds good to me 👍
Yep.
Yeah, I changed that in response to a ci failure. |
devversion
left a comment
devversion
left a comment
There was a problem hiding this comment.
LGTM, aside from Joey's comment on the template file
devversion
added
target: patch
|
Seems like this failed to merge into |
dgp1130
added
target: minor
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
5 participants
@josephperrott @devversion @gregmagolan
The first commit switches the unstamped versions to use the more canonical
0.0.0-PLACEHOLDERand also affects the non-bazel build as it touchespackage.jsons and the legacy build script.The second commit tries to take the approach in the angular components repo for substituting versions and moves the engine substitution versions there.